add min settlement blocks #821
Conversation
prkpndy
left a comment
prkpndy
left a comment
There was a problem hiding this comment.
gg. the logic looks good! have some questions regarding the implementation
| inner: S, | ||
| min_confirmations: u64, | ||
| latest_block: Arc<AtomicU64>, | ||
| _update_task: tokio::task::JoinHandle<()>, |
There was a problem hiding this comment.
can we remove it if not used?
There was a problem hiding this comment.
it's used. it keeps the latest block updating task in memory so that it's not dropped. we should add a comment @byteZorvin
|
|
||
| /// Minimum settlement blocks for L1 sync. | ||
| /// This is the minimum number of blocks that must be settled on L1 before the node can sync/process the messages. | ||
| #[clap(env = "MADARA_MIN_SETTLEMENT_BLOCKS", long, default_value = "10")] |
There was a problem hiding this comment.
nit: can we give it a better name if possible? something like MADARA_MIN_SETTLED_BLOCKS_FOR_MESG_PROCESSING (this is too long but something along these lines)
| /// making the stream implementation much simpler. It's generic and works with any | ||
| /// settlement layer provider. |
There was a problem hiding this comment.
if it can be used with any settlement client, why are we writing it again for starknet client below?
There was a problem hiding this comment.
agreed, we should have one fileter and make it work for both
| pub struct ConfirmationDepthFilteredStream<S, P> { | ||
| inner: S, | ||
| min_confirmations: u64, | ||
| latest_block: Arc<AtomicU64>, | ||
| _update_task: tokio::task::JoinHandle<()>, | ||
| _phantom: std::marker::PhantomData<fn() -> P>, | ||
| } |
There was a problem hiding this comment.
we also have it in ethereum settlement client? why is it duplicated?
There was a problem hiding this comment.
can we create a single struct and a trait and implement that for different clients (if we need to)?
apoorvsadana
left a comment
apoorvsadana
left a comment
There was a problem hiding this comment.
The idea of the filter is good. However, I am unsure if it works as intended.
| Poll::Ready(Some(Ok(event))) => { | ||
| // Check if this event meets the confirmation depth requirement | ||
| let latest = self.latest_block.load(Ordering::Relaxed); | ||
| let threshold = latest.saturating_sub(self.min_confirmations); | ||
|
|
||
| if event.l1_block_number <= threshold { | ||
| return Poll::Ready(Some(Ok(event))); | ||
| } | ||
|
|
||
| // Event doesn't have enough confirmations yet, skip it and continue polling | ||
| tracing::debug!( | ||
| "Skipping event at block {} (needs {} confirmations, latest L1 block: {})", | ||
| event.l1_block_number, | ||
| self.min_confirmations, | ||
| latest | ||
| ); | ||
| // Continue the loop to get the next event | ||
| } |
There was a problem hiding this comment.
are we sure this works? seeems to me that if the depth isn't met then we will not do anything and that event will never come again?
| /// making the stream implementation much simpler. It's generic and works with any | ||
| /// settlement layer provider. |
There was a problem hiding this comment.
agreed, we should have one fileter and make it work for both
| inner: S, | ||
| min_confirmations: u64, | ||
| latest_block: Arc<AtomicU64>, | ||
| _update_task: tokio::task::JoinHandle<()>, |
There was a problem hiding this comment.
it's used. it keeps the latest block updating task in memory so that it's not dropped. we should add a comment @byteZorvin
| /// This uses a background task to periodically update the latest block number, | ||
| /// making the stream implementation much simpler. It's generic and works with any | ||
| /// settlement layer provider. | ||
| pub struct ConfirmationDepthFilteredStream<S> { |
There was a problem hiding this comment.
how can we write a test case for this?
| pub settlement_layer: MadaraSettlementLayer, | ||
|
|
||
| /// Minimum settlement blocks for L1 sync. | ||
| /// This is the minimum number of blocks that must be settled on L1 before the node can sync/process the messages. |
There was a problem hiding this comment.
| /// This is the minimum number of blocks that must be settled on L1 before the node can sync/process the messages. | |
| /// Minimum number of block confirmations an event must have before it can be processed from the settlement layer |
Pull Request type
Please add the labels corresponding to the type of changes your PR introduces:
What is the current behavior?
Resolves: #NA
What is the new behavior?
Does this introduce a breaking change?
Other information